-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updating Translate to Manage Pluralization #38783
Updating Translate to Manage Pluralization #38783
Conversation
@brandonhenry When PR will be ready for review, please add some unit tests for the Pluralization |
Will do @jayeshmangwani ! |
Just wanted to leave an update here that I was working through some other PRs so haven't had that much time to put here. All done on those so this one will get my full attention this week |
thanks for you patience here team. had a lot of back and forth and with local build issues and other things going on, this one kind of fell behind. I've refactored this a lot into a place I think looks good. mind taking a look? @jayeshmangwani @iwiznia |
@jayeshmangwani Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@brandonhenry Above changes will crash the app when we pass the conciergePlaceholderOptions array to localize. Can you test on your side and let us know if the crash is happening on your side, too? iOS: iOS-crash-localize.movweb: |
these lines of code crashing after the changes: App/src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx Lines 201 to 208 in 0b2f55f
|
Please update the localize test that includes the pluralized examples |
sure thing @jayeshmangwani - Let me check again now, thanks for the callout! |
so I did see the issue. fixing this and will have this back before EOD |
Still working through issues here |
…pluralization-example
@iwiznia @jayeshmangwani so the reason this was failing is because we for some reason have an array for this specific translation phrase. Every other translation in this file is a string. I think it should always be a string |
Alrighty, updated the function to handle that one off array translation. I also updated it so that we didn't have to alter phraseKey at all. Now, if you want to have multiple forms for a translation, you just return an object of example:
we still need to pass in count for all the logic to work its magic under the hood. i don't think liner will like the unused variable 🤔 This works now though, no crashes or weirdness and the tests works
|
Made a lot of progress yesterday. Changes pointed out more typing issues. Fixing those up, super close! |
Thanks for the update |
src/libs/Localize/index.ts
Outdated
type PluralFormPhrase = Record<string, string>; | ||
type TranslationPhraseRecord = Record<string, string | number | boolean | null | OnyxEntry<ReportAction> | undefined>; | ||
type TranslationPhraseArg = number | string | undefined | TranslationPhraseRecord; | ||
type TranslationPhraseFunction = (...args: TranslationPhraseArg[]) => string; | ||
type PluralTranslationPhraseFunction = (args: TranslationPhraseArg) => string; | ||
|
||
type PhraseParameters<T> = T extends PluralTranslationPhraseFunction ? [TranslationPhraseArg] : T extends TranslationPhraseFunction ? Parameters<T> : TranslationPhraseArg[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently these types don't work as intended:
translate('common.addressLine') // Should error without parameters
translate('common.personalAddress', {invalid: 'test'}) // Should error with parameters
Taking a look now! I made quite a few changes and finally got errors for missing types in |
@brandonhenry Please let me know if there is anything I can help with here |
This is taking forever to get done.... @brandonhenry can you take it to the finish line at most next week please? |
@iwiznia yes for sure! my apologies, main reason is focusing the smaller PRs i was working on that also seem to be taking forever due to review times... |
…pluralization-example
@iwiznia @jayeshmangwani I'm so sorry 😭 i am living in Houston and we got hit really bad by hurricane beryl. No power since Sunday night. Will keep updated as I also have no service so have to drive out and get it This is really close to done as I worked on it all weekend hoping to finish before beryl but... |
OMG this is epic... |
@Osmon11 indeed. Power is back online as of today! Cleaning this up today / tomorrow |
@brandonhenry can we please take this PR to the finish line today or tomorrow? |
Edit: I'm requesting that this one be transferred to someone else. In hindisght, I should have requested that as soon as I saw we need to change the typing to make this work. My lack of understanding of the system since this is my first expensify ticket, the 3 weeks of power outage / setback and working a 43+ hour full time job set me up for failure here... The code is 90% done in my opinion but my lack of understanding the Expensify codebase fully + my lack of time to invest in doing so is holding this one back. My apologies but I'm available if you have any questions about the current work. Thank you for your understanding |
@shubham1206agra is going to take over as contributor here. |
deleteRates: (count: number) => ({ | ||
zero: `Delete ${count} rates`, | ||
one: `Delete ${count} rate`, | ||
other: `Delete ${count} rates`, | ||
}), | ||
enableRates: (count: number) => ({ | ||
zero: `Enable ${count} rates`, | ||
one: `Enable ${count} rate`, | ||
other: `Enable ${count} rates`, | ||
}), | ||
disableRates: (count: number) => ({ | ||
zero: `Disable ${count} rates`, | ||
one: `Disable ${count} rate`, | ||
other: `Disable ${count} rates`, | ||
}), | ||
areYouSureDelete: (count: number) => ({ | ||
zero: `Are you sure you want to delete ${count} rates?`, | ||
one: `Are you sure you want to delete ${count} rate?`, | ||
other: `Are you sure you want to delete ${count} rates?`, | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference
@iwiznia Can you close this PR? |
Details
Fixed Issues
$ #38614
PROPOSAL: #38614 (comment)
Tests
Navigate to a chat room or report that contains messages with pluralized text related to the maximum number of participants (e.g., "You've selected the maximum number (2) of participants.").
Verify that the pluralized text is displayed correctly based on the count (e.g., "You've selected the maximum number (2) of participants." for count 2, and "You've selected the maximum number (5) of participants." for count 5).
Change the app's language to a different supported language (e.g., Spanish) and verify that the pluralized text is correctly translated and displayed based on the count.
Navigate to a chat room or report that contains messages with pluralized text related to expense counts (e.g., "2 expenses, 1 scanning").
Verify that the pluralized text is displayed correctly based on the count (e.g., "1 expense" for count 1, "2 expenses, 1 scanning" for counts 2 expenses and 1 scanning receipt).
Change the app's language to a different supported language (e.g., Spanish) and verify that the pluralized text is correctly translated and displayed based on the counts.
Navigate to a workspace settings page that shows the "Selected" count for members (e.g., "2 selected").
Verify that the pluralized text is displayed correctly based on the count (e.g., "1 selected" for count 1, "5 selected" for count 5).
Change the app's language to a different supported language (e.g., Spanish) and verify that the pluralized text is correctly translated and displayed based on the count.
Navigate to a workspace settings page that displays options related to distance rates (e.g., "Delete 2 rates", "Enable 3 rates", "Disable 1 rate", "Are you sure you want to delete these 4 rates?").
Verify that the pluralized text is displayed correctly based on the count for each option.
Change the app's language to a different supported language (e.g., Spanish) and verify that the pluralized text is correctly translated and displayed based on the count for each option.
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop